Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(benchpress): create component_benchmark macro #35692

Closed
wants to merge 1 commit into from
Closed

feat(benchpress): create component_benchmark macro #35692

wants to merge 1 commit into from

Conversation

wagnermaciel
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Currently we have a lot of repeat code for writing benchmark tests.

What is the new behavior?

Reduce the amount of duplicate code for these tests by wrapping the boilerplate code in a macro.

Does this PR introduce a breaking change?

  • Yes
  • No

@wagnermaciel
Copy link
Contributor Author

Questions

Is dev-infra the correct place to put this under in .pullapprove.yml?
Is this code located in the correct place?

@wagnermaciel wagnermaciel marked this pull request as ready for review February 26, 2020 21:01
@pullapprove pullapprove bot requested a review from IgorMinar February 26, 2020 21:01
@wagnermaciel wagnermaciel requested review from mhevery and jelbourn and removed request for IgorMinar February 26, 2020 21:02
jelbourn
jelbourn approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wagnermaciel wagnermaciel requested review from kara and removed request for mhevery February 27, 2020 17:53
@mhevery mhevery added the area: build & ci Related the build and CI infrastructure of the project label Feb 27, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 27, 2020
mhevery
mhevery suggested changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment •
edited by wagnermaciel

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • either squash or use fixup! https://dev.to/koffeinfrei/the-git-fixup-workflow-386d. This is a single unit of work and should have single commit when merged.
  • The tools/components/BUILD.bazel file is empty. Is that intentional? If so leave a comment as to why an empty file is needed.
  • Can we have a clear description in the commit message as to the motivation of this change.
  • I don't see any tests (or at least an example of working benchmark) How do I try it/know that it works.

@wagnermaciel wagnermaciel force-pushed the component-benchpress-build-rule branch 4 times, most recently from a405099 to 5ff11b5 Compare March 2, 2020 18:24
@wagnermaciel
Copy link
Contributor Author

@mhevery Done

@wagnermaciel wagnermaciel removed the request for review from kara March 5, 2020 18:02
@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Mar 6, 2020
@wagnermaciel wagnermaciel added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 6, 2020
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Mar 6, 2020
@ngbot
Copy link

ngbot bot commented Mar 6, 2020

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@wagnermaciel
Copy link
Contributor Author

The only TAP failures are already-failing targets

@matsko
Copy link
Contributor

matsko commented Mar 9, 2020

presubmit = http://test/OCL:299857192:BASE:299857197:1583770689304:3497e3f2

@AndrewKushnir
Copy link
Contributor

Hi @wagnermaciel, it looks like this PR has a conflict with master branch. Could you please rebase it and resolve the conflict (and add the merge label back)? Thank you.

@AndrewKushnir AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Mar 11, 2020
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 11, 2020
devversion
devversion approved these changes Mar 11, 2020
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just came across this. Awesome! LGTM

tools/components/component_benchmark.bzl
"""
native.genrule(
name = "copy_default_" + origin + "_file_genrule",
srcs = ["//tools/components/defaults:" + origin],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (for follow-up): It's more idiomatic to use substitutions. e.g.

"//tools/components/defaults:%s" % origin

Same could be done for all string concatenations in the file.

@wagnermaciel wagnermaciel force-pushed the component-benchpress-build-rule branch 6 times, most recently from bb336b0 to 001888c Compare March 18, 2020 17:41
@wagnermaciel
feat(benchpress): create component_benchmark macro
274466c 
* Create component_benchmark macro
* Change class_bindings benchmark to use component_benchmark
@wagnermaciel wagnermaciel force-pushed the component-benchpress-build-rule branch from 001888c to 274466c Compare March 18, 2020 17:49
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit target: major This PR is targeted for the next major release labels Mar 18, 2020
@AndrewKushnir
Copy link
Contributor

@wagnermaciel, FYI given that we've just released 9.1.0-rc.0 and created a new patch branch (9.1.x), I've re-labeled this PR to be "master & patch", so that the changes are included into 9.1.0 release. Thank you.

@mhevery mhevery closed this in 8968b20 Mar 20, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion devversion approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@wagnermaciel @matsko @AndrewKushnir @mhevery @jelbourn @devversion @googlebot